Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: extract @agoric/contracts package #7399

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

refactor: extract @agoric/contracts package #7399

wants to merge 2 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 12, 2023 •
edited by dckc
Loading

closes: #7392
closes: #1656
refs: #XXXX

Description

TODO

  • simplify src/contractSupport to src/support
  • bring in zoe/contracts as src/examples

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

@turadg turadg requested review from dckc and Chris-Hibbert April 12, 2023 19:32
Chris-Hibbert
Chris-Hibbert reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few observations. I'm not going to look closely inside /zoe/ until contracts is moved.

packages/inter-protocol/src/proposals/addAssetToVault.js Outdated
import { deeplyFulfilledObject } from '@agoric/internal';
import { Stable } from '@agoric/vats/src/tokens.js';
import { E } from '@endo/far';
import { parseRatio } from '@agoric/zoe/src/contractSupport/ratio.js';
import { parseRatio } from '@agoric/contracts';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably need to be combined

packages/inter-protocol/src/interest.js Outdated
@@ -1,11 +1,11 @@
import { AmountMath } from '@agoric/ertp';
import { natSafeMath } from '@agoric/zoe/src/contractSupport/index.js';
import { Fail } from '@agoric/assert';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this si the deprecated way. You don't need an import to get assert, just move this below imports.

Suggested change
import { Fail } from '@agoric/assert';
const { Fail } = assert;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the jury's still out on that: #5672

packages/contracts/src/contractSupport/ratio.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to move ratio to internal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no because it needs to be used externally

@turadg turadg force-pushed the 7389-contracts-package branch from 4b2f52c to e146ee5 Compare April 12, 2023 21:46
dckc
dckc reviewed Jul 21, 2023
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LMK when this is rfr.
I'm submitting this to get it off my queue.

@Chris-Hibbert
Copy link
Contributor

zoe/tools/priceAuthorityRegistry.js is used in production, and should be migrated out of /tools/.
zoe/contracts/scaledPriceAuthority.js is used in production, and should not end up with mere demo contracts.
zoe/contractSupport/ contains a mix of tools we use and ones we don't.

@dckc
Copy link
Member

dckc commented Nov 7, 2023

This looks like it closes #1656 too.

@turadg turadg force-pushed the 7389-contracts-package branch from e146ee5 to 2391b00 Compare December 12, 2023 19:25
@turadg turadg mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chris-Hibbert Chris-Hibbert Chris-Hibbert left review comments

@dckc dckc dckc left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@turadg @Chris-Hibbert @dckc